Skip to content

fix(shared): preserve MCPError payload on pickle#2478

Open
MukundaKatta wants to merge 2 commits intomodelcontextprotocol:mainfrom
MukundaKatta:codex/mcp-error-pickle-safe
Open

fix(shared): preserve MCPError payload on pickle#2478
MukundaKatta wants to merge 2 commits intomodelcontextprotocol:mainfrom
MukundaKatta:codex/mcp-error-pickle-safe

Conversation

@MukundaKatta
Copy link
Copy Markdown

Summary

  • make MCPError pickle-safe by reconstructing exceptions from the full ErrorData payload instead of bare Exception args
  • preserve the specialized UrlElicitationRequiredError shape during round-trip reconstruction
  • add regression tests for pickling both MCPError and UrlElicitationRequiredError

Testing

  • python3 -m py_compile src/mcp/shared/exceptions.py tests/shared/test_exceptions.py
  • python3 -m pytest tests/shared/test_exceptions.py -k pickle_roundtrip
  • PYTHONPATH=src python3 -m pytest tests/shared/test_exceptions.py -k pickle_roundtrip

Local note: pytest is blocked in this desktop environment because the available interpreter bottoms out before the repo's required typing features (ImportError on TypeAlias from Python 3.9).

…ror_data

The defensive hasattr/fallback branch was unreachable in practice — every
MCPError subclass inherits from_error_data() from the base class, so the
exc_type.__new__(...) fallback path never fires. Removing it brings coverage
to 100% and keeps the reconstructor to 2 lines.

Behavior unchanged:
- MCPError pickle round-trip
- UrlElicitationRequiredError pickle round-trip (specialized path)
- Arbitrary MCPError subclass pickle round-trip

All 11 tests/shared/test_exceptions.py tests pass.
@MukundaKatta
Copy link
Copy Markdown
Author

CI was failing on coverage (99.98% vs required 100%). The gap was from the defensive hasattr(exc_type, 'from_error_data') / exc_type.__new__(...) fallback branch — unreachable in practice because every MCPError subclass inherits from_error_data from the base class.

Simplified the reconstructor to two lines. Behavior is unchanged for all three cases verified locally (MCPError, UrlElicitationRequiredError, and an arbitrary MCPError subclass); the existing 11 tests in tests/shared/test_exceptions.py all pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant